Decouple bulk load from SHARD_ENCODE_LOCATION_METADATA dependency#13162
Open
saintstack wants to merge 4 commits intoapple:mainfrom
Open
Decouple bulk load from SHARD_ENCODE_LOCATION_METADATA dependency#13162saintstack wants to merge 4 commits intoapple:mainfrom
saintstack wants to merge 4 commits intoapple:mainfrom
Conversation
Bulk load previously required SHARD_ENCODE_LOCATION_METADATA to be enabled, which pulled in the entire DataMoveMetaData persistence machinery. This was unnecessary overhead — the BulkLoadTaskState is already independently persisted at bulkLoadTaskKeys (keyed by range), and the storage server already knows its range when fetching keys. The core change: SS now looks up bulk load task metadata directly by range from bulkLoadTaskKeys, instead of the old indirection path: dataMoveId → DataMoveMetaData → BulkLoadTaskState Changes by component: Storage Server (storageserver.actor.cpp): - Replace getBulkLoadTaskStateFromDataMove() with getBulkLoadTaskStateByRange() in both fetchKeys and fetchShard paths - Remove the gate that disabled conductBulkLoad when dataMoveId was anonymousShardId or invalid - Remove assertions that cross-checked dataMoveId against the task state Data Distributor (DDRelocationQueue.actor.cpp, DataDistribution.cpp/.h): - When SHARD_ENCODE_LOCATION_METADATA is off, generate a proper dataMoveId with LOGICAL_BULKLOAD encoded for bulk load tasks (instead of anonymousShardId) - Add bulk load task validation and dataMoveId assignment path that runs without SHARD_ENCODE_LOCATION_METADATA - Remove the bulkLoadIsEnabled() gate that required SHARD_ENCODE_LOCATION_METADATA - Remove the DDBulkLoadModeMonitorSkipped early return MoveKeys (MoveKeys.cpp): - Add optional dataMoveId parameter to startMoveKeys and finishMoveKeys - When a valid dataMoveId is present, write serverKeysValue(dataMoveId) instead of serverKeysTrue — this encodes the LOGICAL_BULKLOAD type so SS can decode it - Add bulk load phase transitions (Triggered→Running in startMoveKeys, Running→Complete in finishMoveKeys) that were previously only in startMoveShards - Pass dataMoveId through rawStartMovement and rawFinishMovement BulkLoadUtil (BulkLoadUtil.cpp/.h): - New getBulkLoadTaskStateByRange() function that reads bulkLoadTaskKeys directly using krmGetRanges, with the same retry/version logic as the old function Test (BulkLoading.toml): - Changed shard_encode_location_metadata from true to false to exercise the new decoupled path
Contributor
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Contributor
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Contributor
Result of foundationdb-pr on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Contributor
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Contributor
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Contributor
Result of foundationdb-pr on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Contributor
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
added 3 commits
May 6, 2026 22:33
- Assert ranges.size()==1 in fetchShard bulk load path (makes single-range invariant explicit) - Document range mismatch as expected (shard splits within task range are normal; SS only loads keys within its assigned sub-range) - Add detailed comment explaining cancellable=false before prevCleanup (DDQueueValidateError13 race condition)
Contributor
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Contributor
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Contributor
Result of foundationdb-pr-clang on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Contributor
Result of foundationdb-pr on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bulk load previously required SHARD_ENCODE_LOCATION_METADATA to be enabled, which pulled in the entire DataMoveMetaData persistence machinery. This was unnecessary overhead — the BulkLoadTaskState is already independently persisted at bulkLoadTaskKeys (keyed by range), and the storage server already knows its range when fetching keys.
The core change: SS now looks up bulk load task metadata directly by range from bulkLoadTaskKeys, instead of the old indirection path:
dataMoveId → DataMoveMetaData → BulkLoadTaskState
Changes by component:
Storage Server (storageserver.actor.cpp):
Data Distributor (DDRelocationQueue.actor.cpp, DataDistribution.cpp/.h):
MoveKeys (MoveKeys.cpp):
BulkLoadUtil (BulkLoadUtil.cpp/.h):
Test (BulkLoading.toml):